Constrain timestamp in global variables #948
Conversation
LHerskind
force-pushed
the
lh/globals-noir-public
branch
from
53f1386 to
cd2621b
Compare
LHerskind
force-pushed
the
lh/constrain-timestamp-globals
branch
from
3af6fa6 to
058b86d
Compare
| const globalVariables = new GlobalVariables( | ||
| new Fr(config.chainId), | ||
| new Fr(config.version), | ||
| new Fr(1 + i), | ||
| Fr.ZERO, | ||
| new Fr(await rollup.read.lastBlockTs()), |
There was a problem hiding this comment.
Why do we want the ts of the last block here? Do we not want the current timestamp?
There was a problem hiding this comment.
The ts of the last block is the minimum value that will work. With the current timestamp, you have the issue of dealing with anvil not progressing time as "outside" so this was just an easy way to pick a meaningful value.
| * @returns The global variables for the given block number. | ||
| */ | ||
| public async buildGlobalVariables(blockNumber: Fr): Promise<GlobalVariables> { | ||
| const lastTimestamp = new Fr(await this.reader.getLastTimestamp()); |
There was a problem hiding this comment.
See above, also hinted in the comments above the function.
| revert Errors.Rollup__TimestampInFuture(); | ||
| } | ||
|
|
||
| // @todo @LHerskind consider if this is too strict |
There was a problem hiding this comment.
is there a relevant issue for us to come back to this later or is the plan to hash this out now?
There was a problem hiding this comment.
Have not created an issue for it. Will add it to #824.
| /** | ||
| * The API key of the ethereum host. | ||
| */ | ||
| apiKey?: string; |
There was a problem hiding this comment.
isn't this on sequencer config already?
There was a problem hiding this comment.
Yes, so it is just to not require the entire config to be passed to the builder. If you look at the other configs, you will see similar with values that are "shared" between them, so the full config makes up the union of them.
Description
Fixes #830. Built on top of #917.
Checklist: